One point that I noticed while developing #1478 (and checking how my draft of that branch interacted with this PR): it'd be good for this commit:
e268041ca content: Handle vertical offset spans in KaTeX content

to get split up like so:

• First, an NFC prep commit introduces the distinction between KatexNode and KatexSpanNode. At this stage, the latter is the only subclass of the former.
• Then another commit makes the substantive changes this commit is about, including introducing the sibling subclasses KatexVlistNode and KatexVlistRowNode.

One reason that'd be useful is that the split between KatexNode and KatexSpanNode is somewhat nontrivial in itself: some of the references to KatexNode continue to say KatexNode, while others switch to saying KatexSpanNode, so the commit is expressing meaningful information by its choices of which references go which way.

One other possibility for this to be logged is when _getEm returns null. I think for debugging purpose, including the value expression might be helpful.

Let's perhaps have a short dartdoc on what this does and what null means, since the if (…Em != null) continue;'s make this return value quite relevant.

I think it's a good idea to separate them into groups; to add on this, how about having a short comment before each group explaining how we group them together (like we do with design variables)?

Hmm, !styles.marginRightEm!.isNegative seems a bit contrived. I think isNegative negated is not as clear as >= 0. However, do we need the margin when marginRightEm is 0?

Having tests for this (the case when there are both inline styles and others) might be useful. Not sure how common that is.

looks like this can be replaced with _KatexNodeList(nodes: row.nodes)

_KatexNodeList also seems helpful here.

Because the name implies that the margin should be negative, maybe we should also add assertions to ensure that that is true.

Neat solution! A bit tricky to understand. I think we can use QueueList (and with .addFirst) to avoid the hassle of reversing and unreversing the list.

It will be helpful to have tests for this.

Do we consider this complete after the final commit ("content: Support negative right-margin on KaTeX spans")? I'm not too sure if something else is left to be done here.

This seems to contradict with the earlier assumption that vlist elements contain only the height style.

nit: indentation

Interesting setup! This resembles the golden tests that use images instead of offset/size values to verify rendering results.

nit: newline at the end

We have a similar thing in emoji_reactoins_test.dart:

  Future<void> prepare() async {
    addTearDown(testBinding.reset);
    await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
    store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

    await store.addUser(eg.selfUser);

    // TODO do this more centrally, or put in reusable helper
    final Future<ByteData> font = rootBundle.load('assets/Source_Sans_3/SourceSans3VF-Upright.otf');
    final fontLoader = FontLoader('Source Sans 3')..addFont(font);
    await fontLoader.load();
  }

Sharing the helper is not necessary for this PR, but it looks like something we can extract to reuse in the future.

nit: should this be .msupsub?

Not entirely sure why, but math nested in headings doesn't seem to be visible:

Another issue is that the math content seems to get scaled more than normal text, and the smaller the text is (in normal font size), the more scaled it gets.

normal	larger font

nit: indentation

I think this assertion is based on the broader assumption that some inline styles never co-occur to vertical-align or height. This seems like something we can comment on.

+    // TODO: Re-enable this test after adding support for parsing
+    //       `vertical-align` in inline styles. Currently it fails
+    //       because `strut` span has `vertical-align`.
+    //
+    // testWidgets('displays KaTeX content with different delimiter sizing', (tester) async {

Instead of commenting out the whole test (which makes the diff big), pass skip: true. For examples, see git grep -A2 skip: test/.

The other reason skip: is better than commenting out is that it makes the test case still exist in the test framework and get counted; the total number of skipped tests is printed after the number of passed and failed tests. That's helpful for occasionally sweeping to confirm we don't have tests accidentally left skipped that might point to live bugs.

+  // TODO: Re-enable this test after adding support for parsing
+  //       `vertical-align` in inline styles. Currently it fails
+  //       because `strut` span has `vertical-align`.
+  //
+  // static const mathBlockKatexDelimSizing = ContentExample(

Similarly, use skip: true instead of commenting.

For this test, that requires a bit of setup to make this file's helpers and the "all content examples are tested" check handle the skip: argument. I've just pushed a small added commit at the end of this PR branch to do that:
f9a79d875 content test [nfc]: Enable skips in testParseExample and testParse

So please rebase that to earlier in the branch as needed, and then write:

  testParseExample(ContentExample.mathBlockKatexNestedSizing, skip: true);

Same comment as at #1609 (comment) , from when this commit was in a previous version of #1609.

(But now in this PR presumably I'll see what motivated trying this change.)

Hmm, well, I don't see that yet. :-)

What happens if you drop this commit?

This commit is pre-req for 4bea0c3065417ffbb247818e50b809c294b372e3.

Allowing to get the rect for Katex widget without the Center(SingleChildScrollViewWithScrollbar( widgets affecting it, otherwise the offsets would take into account the whole test window width.

         (ContentExample.mathBlockKatexSizing, skip: false, [
-          ('1', Offset(0.00, 2.24), Size(25.59, 61.00)),
-          ('2', Offset(25.59, 9.90), Size(21.33, 51.00)),
-          ('3', Offset(46.91, 16.30), Size(17.77, 43.00)),
-          ('4', Offset(64.68, 21.63), Size(14.80, 36.00)),
-          ('5', Offset(79.48, 26.07), Size(12.34, 30.00)),
-          ('6', Offset(91.82, 29.77), Size(10.28, 25.00)),
-          ('7', Offset(102.10, 31.62), Size(9.25, 22.00)),
-          ('8', Offset(111.35, 33.47), Size(8.23, 20.00)),
-          ('9', Offset(119.58, 35.32), Size(7.20, 17.00)),
-          ('0', Offset(126.77, 39.02), Size(5.14, 12.00)),
+          ('1', Offset(334.02, 2.24), Size(25.59, 61.00)),
+          ('2', Offset(359.61, 9.90), Size(21.33, 51.00)),
+          ('3', Offset(380.94, 16.30), Size(17.77, 43.00)),
+          ('4', Offset(398.71, 21.63), Size(14.81, 36.00)),
+          ('5', Offset(413.52, 26.07), Size(12.34, 30.00)),
+          ('6', Offset(425.87, 29.77), Size(10.29, 25.00)),
+          ('7', Offset(436.15, 31.62), Size(9.26, 22.00)),
+          ('8', Offset(445.41, 33.47), Size(8.23, 20.00)),
+          ('9', Offset(453.64, 35.32), Size(7.20, 17.00)),
+          ('0', Offset(460.84, 39.02), Size(5.14, 12.00)),
         ]),

Hmm, I see.

OK, please file a follow-up issue to track the bug described in #1609 (comment) . Then this code can just get a TODO comment mentioning that issue.

Hmm, I think wrapping SingleChildScrollViewWithScrollbar in Directionality again should fix this potential bug.

diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart
index 396bbbba..27426792 100644
--- a/lib/widgets/content.dart
+++ b/lib/widgets/content.dart
@@ -821,10 +821,12 @@ class MathBlock extends StatelessWidget {
     }
 
     return Center(
-      child: SingleChildScrollViewWithScrollbar(
-        scrollDirection: Axis.horizontal,
-        child: _Katex(
-          nodes: nodes)));
+      child: Directionality(
+        textDirection: TextDirection.ltr,
+        child: SingleChildScrollViewWithScrollbar(
+          scrollDirection: Axis.horizontal,
+          child: _Katex(
+            nodes: nodes))));
   }
 }

Sure, that sounds good. Better than leaving a follow-up issue, in fact :-)

Added this fix in 9de7110ff91fa52f643ca3672dbb09e3ad86a9cc.

nit: this name feels a bit general for the public namespace (and fortunately it has only a few mentions, so there's minimal cost to a little verbosity)

nit: "work around" for the verb

Huh, does that do it? I wouldn't expect a quote to interact with this bug — those don't change the text size.

(Rereading, I'm not sure that comment by Zixuan was about the bug this fixes, rather than a different issue. What I'm hoping for in this comment is a test case that would fail if we didn't scale inline TeX to match the surrounding font size. Put another way, a test case that matches what's in the commit message:

content: Scale inline KaTeX content based on the surrounding text

This applies the correct font scaling if the KaTeX content is
inside a header.

)

(cont'd from #1452 (comment))

This sort of test may work in that it breaks if the bug is re-introduced — but then the next key thing a test needs to do, when it breaks due to a regression, is to convince that future developer that there really is a regression, and to help them work out what it is. If it doesn't convince them, they're most likely to just update the test to expect the new values, and the test didn't succeed in preventing the regression.

So a test is most effective if it can tell a crisp story about what it's checking, and why its expectations really should be expected. These test cases centered on a list of numbers typically aren't good at that, though they can still be the best available strategy for some things.

For this test case, the potential regression we're looking for is that the font size doesn't stay in sync with the font size of the surrounding text. This test file has a helper function for just that sort of check: checkFontSizeRatio. Can you write a test using that? Then that would make it a lot clearer what the narrative is.

Added the checkFontSizeRatio test squashed in 05b51db4ae982a66eada184be2da7e374dfb22c6.

… Also a test that checks a given commit works correctly should be squashed into the same commit 🙂 (or sometimes should come before and get updated in the main commit), rather than come after it.

Huh, it's interesting that in this commit:

content: Scale inline KaTeX content based on the surrounding text

This applies the correct font scaling if the KaTeX content is
inside a header.

the changes that happen in the tests are entirely about changing the vertical offsets of these characters — no changes to the sizes, nor the horizontal offsets. Do you understand why the vertical offsets are changing when the sizes aren't?

In any case, the lack of size-related test changes seems to confirm that this commit (which is about getting the sizes right) still needs testing.

The change in offset is because of TextLeadingDistribution.even. Moved it to a separate commit c748bcef5f4b3a03ca14c4f5a5aace47759ab075.

nit: put the margin computation above next to the spot where margin is used, here

Cool, this is a good idea for making sure we don't forget fontSize ?? in any em references below.

Cool, thanks. Does that mean that the marginLeftEm code here never gets exercised? In that case let's have an assert to that effect.

content: Update base KaTeX text style to be explicit

This fixes two potential bugs:

- First about `leadingDistribution` where previously it was taking
the default value of `TextLeadingDistribution.proportional`, now it
uses `TextLeadingDistribution.even` which seems to be the default
strategy used by CSS, and it doesn't look like `katex.scss`
overrides it.

The vertical offsets being updated in tests are because of this fix.

This one is an actual bug, right? (Not just potential.) The changes in those vertical offsets demonstrate it was having a real effect — things were sitting a little too high, and also not quite aligned correctly with each other.

It is a small bug, though: at least in the test data, the differences are all less than one logical pixel (so up to a couple of physical pixels).

- Second about `textBaseline` where on some locale systems the
default value for this could be `TextBaseline.ideographic`, and for
KaTeX we always want `TextBaseline.alphabetic`.

This one I guess is "potential" because I'm not sure whether the two baselines end up having a different effect with the fonts that KaTeX uses.

OK, sounds good.

Hmm, I see.

OK, please file a follow-up issue to track the bug described in #1609 (comment) . Then this code can just get a TODO comment mentioning that issue.

Cool, this looks good.